-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(model): #35 add an interface of integrals of motion #54
base: main
Are you sure you want to change the base?
feat(model): #35 add an interface of integrals of motion #54
Conversation
Coverage Report (73a702c, 3.12, ubuntu-latest)
|
Coverage Report (73a702c, 3.11, macos-latest)
|
Coverage Report (73a702c, 3.12, macos-latest)
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are moving initial conditions to a dedicated module, let's also move the following lines to a dedicated module (maybe model.py
or something).
I feel most people won't look into __init__.py
unless it is related to __all__
.
Maybe we only do imports in __init__.py
. Something like
from hamilflow.models.harmonic_oscillator.model import SimpleHarmonicOscillator, DampdHamonicOscillator
__all__ = ["SimpleHarmonicOscillator", "DampdHamonicOscillator"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to separate engineering and scientific efforts to different issues and MRs, so we can streamline iterations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
], | ||
) | ||
def test_output( | ||
self, omega: float, input: dict[str, float], expected: dict[str, float] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see what is not compatible with py39... It is expected: dict[str, float]
. Should be expected: Dict[str, float]
, where Dict
is imported from typing
.
@@ -155,6 +147,7 @@ def __init__( | |||
system: Dict[str, float], | |||
initial_condition: Optional[Dict[str, float]] = {}, | |||
): | |||
initial_condition = parse_ic_for_sho(system["omega"], **initial_condition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I am not happy with the syntax system["omega"]
. It breaks the design and steals data from the system
dictionary. I guess we need to improve the design at some point. @emptymalei
phi: float = Field(default=0.0) | ||
|
||
|
||
def parse_ic_for_sho(omega: float, **kwargs: Any) -> Dict[str, float]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one looks quite bad. Can we reduce the complexity by settle on one certain kind of convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't aim to generate a good interface for users to use. As long as we can set the convention for ourselves, I am okay with anyone of the three choices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that
Let us sleep over it a bit and then decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that (E,t0) is associated with the first integral of the system. First integrals, or constants of motion, have special and physical meaning in the framework of analytical mechanics. This is in contrast with the phase ϕ.
Let us sleep over it a bit and then decide.
Can we just keep
No description provided.